Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant prompts and inappropriate retreat options. Bump version and fix old version processing #1621

Closed
wants to merge 30 commits into from

Conversation

simon33-2
Copy link
Contributor

Hmm, having a small amount of trouble with git. Once a commit has been pushed I'm not sure how to remove it from a given branch, so I killed "RemovePrompts2"

Previous attempts are #1599 & #1615.

Basically, this resolves most or all of #1270. Also removes a redundant prompt for submerging subs against only air units.

This PR and #1583 break serialisation in certain cases so move the version number on and establish a new file for the latest version. If there is a better way of doing this then I am all ears.

The major reason why isn't it achievable to download the actual source file which contains the version number is because that needs to be merged in two phases. The build will fail if code which refers to the source is merged at the same time as the source being updated, if you follow me. This is a one time problem though. If this is merged, I reckon just update EngineVersionProperties to refer to GameEnginePropertyFileReader. Then the version is stored in a single place and a jar file knows what version it is, if not which build.

Simon and others added 25 commits March 24, 2017 22:02
Don't ask for submerge or retreat when only defenseless transports remain
Don't ask for submerge or retreat when only defenseless transports remain
…384ecfdc54d6096f16cc56ea8360044ca993' into RemovePrompts2

Old Jar 1.9.0 made with above commit 4ca993
Copy link
Member

@ssoloff ssoloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observations...

@@ -815,9 +833,24 @@ private void pushFightLoopOnStack(final boolean firstRun) {

@Override
public void execute(final ExecutionStack stack, final IDelegateBridge bridge) {
if( Match.someMatch( m_attackingUnits, Matches.unitHasAttackValueOfAtLeast(1) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a Checkstyle whitespace violation (just run the Eclipse formatter to fix it).

@@ -0,0 +1,4 @@
LATEST=1.9.1.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the background on why we need a latest_version_new.properties?

@@ -31,6 +32,9 @@ protected GameEnginePropertyFileReader(final File propertyFile) {

@Override
public String readProperty(final GameEngineProperty propertyKey) {
if (propertyKey == GameEngineProperty.ENGINE_VERSION) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug? It looks like object equality is being checked instead of verifying values. ie: this if statement will only be true when propertyKey and GameEnginerProperty.ENGINE_VERSION are the same instance.

@DanVanAtta
Copy link
Member

@simon33-2 some blockers:

  • multiple changes in same PR. It's going to be difficult to get everything discussed and tested.
  • It's very significant to upgrade game compatibility version and it is impactful to the players. This update can potentially be blocked until we have enough changes queued to justify a version upgrade. (One reason why eliminating compatibility issues is so important).
  • The version number problems/handling should be discussed a good bit more. I'm not quite sold on adding back in a new place where we read game engine version number and am not clear on the intent of the new property file.

@simon33-2 given the above, what are your thoughts and intent on how to move forward?

@simon33-2
Copy link
Contributor Author

simon33-2 commented Apr 10, 2017

We need to go to 1.9.1 anyway. We already have at least one change in the master code line which can break serialisation in some cases. Besides, this improvement is worth breaking backwards compatibility.

Re: latest_version file.
We cannot update the current file at present because that causes all existing instances of 1.9.0 up to the currently released (not just built) ones to hang.

My idea of going to a new file would be that would allow 1.9.1 users to be notified when that version is deprecated. However, there is a better idea which is possible; I don't know how this was handled in 1.8 although it does appear that the jar file did indeed know what version it was. By incorporating the version in the source we can completely remove the latest_version* in later versions. If you are worrying about this, I think one option is to just have a 1.9.1 think that the latest version is 1.9.0 in the first build and then quickly update it before deploying to triplea-game.org or marking it as the latest release. Would that make you happy about this aspect? Keen to hear your thoughts.

Re: GameEnginerProperty.ENGINE_VERSION
I don't think it is a bug since you should be using the constant. In fact, it is illegal to not use the constant isn't it?

@DanVanAtta
Copy link
Member

DanVanAtta commented Apr 10, 2017

I see, I'm not that opposed to going to 1.9.1

We are in a tough spot though since the intended update mechanism is broken. My hope/intent was we could work out what was needed to ensure we could do that upgrade. The game is supposed to notify you when there is a new version available and prompt you to download it.

The engine actually does know which version it is by parsing the game_engine.properties file. Note that the game engine prints out the current version on the load game menus.

The new file is an interesting idea. We will fail to notify users of the new version going that route. Since that is broken anyways, i can understand the work around. I was thinking if we are explicit enough with some lobby messaging and let enough time go by with a patched version available for release, that those who are 'stuck' will be few and will hopefully re-download.

@simon33-2
Copy link
Contributor Author

simon33-2 commented Apr 10, 2017

So which way are you leaning:

  1. New file
  2. Non functional latest_version check
  3. something else?

We need to avoid having nearly 100% of installed users hang IMO. When we get the majority of users with a functional latest version check, we can start to think about breaking the system for those old users.

@DanVanAtta
Copy link
Member

DanVanAtta commented Apr 10, 2017

New file would have the problem that it would be difficult to migrate off of it.

If we had a lot of time, I would lean towards two, non functional. Migrating to a new file is a bit more effort, but helps with a shorter term rollout. New file and planning to stay with that file is perhaps the best migration path available.

@simon33-2
Copy link
Contributor Author

I'm inclined to agree, given that I'm suggesting a single point of truth solution. I'll push that up shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants